Kernel: T8914: add support for 2.5G pluggables on BCM57810S - #1227
Conversation
c-po
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used🔍 Remote MCP Context7Relevant kernel-doc facts for this review:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds 2.5G pluggable handling in Changesbnx2x 2.5G pluggable support
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch (1)
98-98: 🗄️ Data Integrity & Integration | 🟠 Major
Multi-Arch: sameis wrong here (scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch:91-98).linux-perf-${KERNELRELEASE}installs arch-specificperfbinaries under shared paths, so co-installing different architectures will conflict. Drop the field, or useforeignonly if cross-arch dependency resolution is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch` at line 98, The package metadata in the linux-perf patch is marking the package as Multi-Arch: same, but the linux-perf-${KERNELRELEASE} package installs arch-specific binaries into shared paths and cannot be safely co-installed across architectures. Update the control data in the patch that generates the package to remove Multi-Arch: same, or switch to Multi-Arch: foreign only if cross-arch dependency resolution is actually needed, and keep the change aligned with the package definition for linux-perf-${KERNELRELEASE}.scripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patch (3)
59-59: 🎯 Functional Correctness | 🟠 MajorRemove or wire the IPv4
link_filtersysctl.
include/linux/inetdevice.h:139andnet/ipv4/devinet.c:2652expose/proc/sys/net/ipv4/conf/*/link_filter, but nothing in-tree readsIN_DEV_LINKFILTER(). The only runtime consumer isnet/ipv6/route.c:145-160. Either add an IPv4 consumer or drop the IPv4 sysctl/docs inDocumentation/networking/ip-sysctl.rst:2042.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patch` at line 59, The new IN_DEV_LINKFILTER macro adds an IPv4 sysctl that is exposed by the existing inetdevice/devinet plumbing, but there is still no in-tree IPv4 code path using it. Either wire this setting into an IPv4 consumer that reads IN_DEV_LINKFILTER from the relevant IPv4 routing/device logic, or remove the IPv4 exposure entirely by undoing the sysctl/doc additions in the inetdevice, devinet, and ip-sysctl pieces so the interface only exists where it is actually used.
145-150: 🩺 Stability & Availability | 🟠 MajorGuard
__in6_dev_get()before dereferencing inscripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patch:145-150.
__in6_dev_get(dev)can returnNULLwhen IPv6 state is absent or torn down, so->cnf.link_filterhere can NULL-deref during route lookup. Cache theinet6_dev, return 0 if missing, and readlink_filterwithREAD_ONCE().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patch` around lines 145 - 150, The rt6_link_filter helper dereferences __in6_dev_get(dev) without checking for NULL, so update this function to first cache the inet6_dev pointer from __in6_dev_get, return 0 when it is absent, and then read cnf.link_filter safely with READ_ONCE() before evaluating the netif_running/netif_carrier_ok conditions.
127-133: 🎯 Functional Correctness | 🟡 MinorClamp
link_filterto 0–2 in both sysctls
Documentation/networking/ip-sysctl.rstdocuments only0/1/2, butproc_dointvecstill accepts negatives and values above2. That makes negative values behave like mode1and>2behave like mode2. Useproc_dointvec_minmaxwithSYSCTL_ZERO/SYSCTL_TWOfor both the IPv6 entry here and the IPv4DEVINET_SYSCTL_RW_ENTRY(LINKFILTER, "link_filter")path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patch` around lines 127 - 133, Clamp the link_filter sysctls to the documented 0–2 range by replacing proc_dointvec with proc_dointvec_minmax for the IPv6 entry in the ipv6_devconf table and the IPv4 DEVINET_SYSCTL_RW_ENTRY(LINKFILTER, "link_filter") path. Use SYSCTL_ZERO and SYSCTL_TWO as the bounds so values below 0 or above 2 are rejected instead of being treated like valid modes.
🧹 Nitpick comments (1)
scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch (1)
94-94: 🚀 Performance & Scalability | 🔵 TrivialUse Debian shlib substitution for perf deps
scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch:94should useDepends: ${shlibs:Depends}, ${misc:Depends}instead of listinglibpfm4, libslang2, libtraceevent1directly; that keeps the package metadata aligned with the actual linked libraries and ABI changes.♻️ Suggested direction
-Depends: libpfm4, libslang2, libtraceevent1 +Depends: ${shlibs:Depends}, ${misc:Depends}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch` at line 94, The perf package dependency declaration is hardcoding shared library names instead of using Debian shlib substitution. Update the dependency entry in the linux-perf package patch to use ${shlibs:Depends} and ${misc:Depends} rather than listing libpfm4, libslang2, and libtraceevent1 directly, so the packaging metadata stays aligned with the symbols and ABI actually used by the perf package build.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@scripts/package-build/linux-kernel/patches/kernel/0003-Kernel-T8914-add-support-for-2.5G-pluggables-on-BCM5.patch`:
- Line 31: Update the `mask_tx_fault` contract in the patch so it is clearly and
consistently treated as a per-port bitmask across the relevant code paths,
especially in the logic around the `mask_tx_fault` field and its read/print
handling. Adjust the surrounding checks and any user-facing description so
`mask_tx_fault=1` is understood as masking only port 0 rather than all TX fault
detection, and make the `port_*`/TX fault handling code reflect bitmask
semantics instead of boolean semantics.
- Around line 51-59: The 2.5G enablement check in the fiber-mode path is too
permissive because `speed_cap_mask & (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G)` allows either capability alone to pass.
Update the condition in the `fiber_mode` / `phy->req_line_speed == SPEED_2500`
block (and the matching later path) so the 2.5G programming via
`bnx2x_cl45_write` only runs when both the 1G and 2.5G capability bits are
present, matching the “1G advertisment” requirement.
---
Outside diff comments:
In
`@scripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patch`:
- Line 59: The new IN_DEV_LINKFILTER macro adds an IPv4 sysctl that is exposed
by the existing inetdevice/devinet plumbing, but there is still no in-tree IPv4
code path using it. Either wire this setting into an IPv4 consumer that reads
IN_DEV_LINKFILTER from the relevant IPv4 routing/device logic, or remove the
IPv4 exposure entirely by undoing the sysctl/doc additions in the inetdevice,
devinet, and ip-sysctl pieces so the interface only exists where it is actually
used.
- Around line 145-150: The rt6_link_filter helper dereferences
__in6_dev_get(dev) without checking for NULL, so update this function to first
cache the inet6_dev pointer from __in6_dev_get, return 0 when it is absent, and
then read cnf.link_filter safely with READ_ONCE() before evaluating the
netif_running/netif_carrier_ok conditions.
- Around line 127-133: Clamp the link_filter sysctls to the documented 0–2 range
by replacing proc_dointvec with proc_dointvec_minmax for the IPv6 entry in the
ipv6_devconf table and the IPv4 DEVINET_SYSCTL_RW_ENTRY(LINKFILTER,
"link_filter") path. Use SYSCTL_ZERO and SYSCTL_TWO as the bounds so values
below 0 or above 2 are rejected instead of being treated like valid modes.
In
`@scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch`:
- Line 98: The package metadata in the linux-perf patch is marking the package
as Multi-Arch: same, but the linux-perf-${KERNELRELEASE} package installs
arch-specific binaries into shared paths and cannot be safely co-installed
across architectures. Update the control data in the patch that generates the
package to remove Multi-Arch: same, or switch to Multi-Arch: foreign only if
cross-arch dependency resolution is actually needed, and keep the change aligned
with the package definition for linux-perf-${KERNELRELEASE}.
---
Nitpick comments:
In
`@scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch`:
- Line 94: The perf package dependency declaration is hardcoding shared library
names instead of using Debian shlib substitution. Update the dependency entry in
the linux-perf package patch to use ${shlibs:Depends} and ${misc:Depends} rather
than listing libpfm4, libslang2, and libtraceevent1 directly, so the packaging
metadata stays aligned with the symbols and ABI actually used by the perf
package build.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bd54cabf-0685-44d4-987c-6a38b94a40f9
📒 Files selected for processing (3)
scripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patchscripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patchscripts/package-build/linux-kernel/patches/kernel/0003-Kernel-T8914-add-support-for-2.5G-pluggables-on-BCM5.patch
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
🔍 Remote MCP Context7
Extra review context
-
Linux kernel docs distinguish admin state from carrier state:
IFF_UPmeans the interface is administratively up,IFF_LOWER_UPreflectsnetif_carrier_on(), andIFF_RUNNINGis kept for backward compatibility. The docs also note thatip linkshowsUPvsNO-CARRIERfor these conditions. -
IPv6/IP sysctl docs already use per-interface configuration under
conf/interface/*, withconf/default/*for new interfaces andconf/all/*to update all interfaces. So adding a per-interfacelink_filtersetting matches the existing sysctl layout (inference).
🔇 Additional comments (5)
scripts/package-build/linux-kernel/patches/kernel/0002-build-linux-perf-package.patch (2)
36-48: LGTM!
53-58: LGTM!Also applies to: 64-76
scripts/package-build/linux-kernel/patches/kernel/0001-linkstate-ip-device-attribute.patch (2)
37-46: LGTM!Also applies to: 71-71, 83-95, 115-119
156-161: LGTM!scripts/package-build/linux-kernel/patches/kernel/0003-Kernel-T8914-add-support-for-2.5G-pluggables-on-BCM5.patch (1)
231-233: 🎯 Functional CorrectnessNo remaining
MDIO_AN_REG_8727_MISC_CTRLreferences.
| + /* Note that 2.5G works only when used with 1G advertisment */ | ||
| + if (fiber_mode && (phy->req_line_speed == SPEED_2500) && | ||
| + (phy->speed_cap_mask & | ||
| + (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G | | ||
| + PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G))) { | ||
| + bnx2x_cl45_write(bp, phy, MDIO_WC_DEVAD, | ||
| + MDIO_WC_REG_SERDESDIGITAL_MISC1, | ||
| + 0x6010); | ||
| + } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require the documented 1G + 2.5G capability combination.
Line 52 and Line 102 use speed_cap_mask & (1G | 2_5G), so either bit alone enables the 2.5G path. That contradicts Line 51’s “only when used with 1G advertisment” constraint and can configure 2.5G for an incomplete capability mask.
Proposed fix
- (phy->speed_cap_mask &
- (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
- PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G))) {
+ ((phy->speed_cap_mask &
+ (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
+ PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G)) ==
+ (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
+ PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G))) {- (phy->speed_cap_mask &
- (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
- PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G))) {
+ ((phy->speed_cap_mask &
+ (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
+ PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G)) ==
+ (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
+ PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G))) {Also applies to: 102-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@scripts/package-build/linux-kernel/patches/kernel/0003-Kernel-T8914-add-support-for-2.5G-pluggables-on-BCM5.patch`
around lines 51 - 59, The 2.5G enablement check in the fiber-mode path is too
permissive because `speed_cap_mask & (PORT_HW_CFG_SPEED_CAPABILITY_D0_1G |
PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G)` allows either capability alone to pass.
Update the condition in the `fiber_mode` / `phy->req_line_speed == SPEED_2500`
block (and the matching later path) so the 2.5G programming via
`bnx2x_cl45_write` only runs when both the 1G and 2.5G capability bits are
present, matching the “1G advertisment” requirement.
Add the well-known JAMESMTL kernel module patch for bnx2x to advertise 2.5Gbit/s capabilities on Broadcom NetXtreme2-X cards with BCM57810S chipset. This is useful for ISP GPON access networks that use 2.5Gbit/s pluggables and need the NIC to negotiate beyond 1000baseT/Full, avoiding the 940Mbit/s practical cap on overprovisioned 1G services. References: * https://hack-gpon.org/broadcom-57810s/ * https://github.com/JAMESMTL/snippets/blob/dceb2fee74d80c66d/bnx2x/patches/bnx2x_warpcore_8727_2_5g_sgmii_txfault.patch
sever-sever
left a comment
sever-sever
left a comment
There was a problem hiding this comment.
Add support for BCM57810S NICs KERNEL patch
jestabro
left a comment
jestabro
left a comment
There was a problem hiding this comment.
Add JAMESMTL patch for BCM57810S chipset.
|
Tick the box to add this pull request to the merge queue (same as
|
vyos-bot
Bot
added
mirror-initiated
dmbaturin
left a comment
dmbaturin
left a comment
There was a problem hiding this comment.
If the patch is reviewed and tested by the Linux community, I have nothing against including it.
4 participants
@mergify depends #1226
Change summary
Add the well-known JAMESMTL kernel module patch for bnx2x to advertise 2.5Gbit/s
capabilities on Broadcom NetXtreme2-X cards with BCM57810S chipset.
This is useful for ISP GPON access networks that use 2.5Gbit/s pluggables and
need the NIC to negotiate beyond 1000baseT/Full, avoiding the 940Mbit/s
practical cap on overprovisioned 1G services.
References:
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Checklist: